Skip to content

Comments

Remove const FLAGS.#152791

Merged
rust-bors[bot] merged 2 commits intorust-lang:mainfrom
nnethercote:rm-const-FLAGS
Feb 23, 2026
Merged

Remove const FLAGS.#152791
rust-bors[bot] merged 2 commits intorust-lang:mainfrom
nnethercote:rm-const-FLAGS

Conversation

@nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Feb 18, 2026

View all comments

The performance wins provided by these types are meagre, and I don't think they justify the code complexity they introduce.

r? @Zalathar

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 18, 2026
@nnethercote
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

rust-bors bot pushed a commit that referenced this pull request Feb 18, 2026
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 18, 2026
@Zalathar
Copy link
Member

I wonder if this PR's approach will preserve some of the benefits of static flags via inlining. It's certainly nicer than having FLAGS everywhere.

@rust-bors
Copy link
Contributor

rust-bors bot commented Feb 18, 2026

☀️ Try build successful (CI)
Build commit: 744269b (744269beec5ae0238f5da62161b8168c2e7f8956, parent: 8387095803f21a256a9a772ac1f9b41ed4d5aa0a)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (744269b): comparison URL.

Overall result: ❌ regressions - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
0.3% [0.1%, 0.9%] 42
Regressions ❌
(secondary)
0.4% [0.1%, 1.0%] 78
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.3% [0.1%, 0.9%] 42

Max RSS (memory usage)

Results (primary 2.4%, secondary -0.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
2.4% [1.8%, 3.6%] 3
Regressions ❌
(secondary)
3.3% [1.8%, 6.0%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.6% [-4.0%, -3.2%] 3
All ❌✅ (primary) 2.4% [1.8%, 3.6%] 3

Cycles

Results (primary 3.0%, secondary 3.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
3.0% [2.4%, 4.1%] 6
Regressions ❌
(secondary)
3.3% [2.1%, 4.5%] 29
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.0% [2.4%, 4.1%] 6

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 480.396s -> 481.316s (0.19%)
Artifact size: 397.88 MiB -> 397.75 MiB (-0.03%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 18, 2026
@rust-bors

This comment has been minimized.

@nnethercote
Copy link
Contributor Author

Just enough of a regression that it's hard to justify, alas. I see now that SemiDynamicQueryDispatcher and QueryVTable could be merged if FLAGS went away.

Relatedly, I found the following functions take a FLAGS generic param but don't actually use it:

  • mk_cycle
  • handle_cycle_error
  • cycle_error
  • wait_for_query
  • try_load_from_disk_and_cache_in_memory
  • query_key_hash_verify
  • try_load_from_on_disk_cache_inner

They can be changed to take a QueryVTable instead of a SemiDynamicQueryDispatcher, though then some methods on the latter need to be moved to the former in such a way that's it's not a uniform facade type.

@nnethercote
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

rust-bors bot pushed a commit that referenced this pull request Feb 19, 2026
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 19, 2026
@rust-log-analyzer

This comment has been minimized.

@rust-bors
Copy link
Contributor

rust-bors bot commented Feb 19, 2026

☀️ Try build successful (CI)
Build commit: 06d3be4 (06d3be4e20551ccb3f23a19b0d08dabb3542e3fa, parent: e0cb264b814526acb82def4b5810e394a2ed294f)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (06d3be4): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
0.3% [0.2%, 0.5%] 14
Regressions ❌
(secondary)
0.3% [0.1%, 0.6%] 33
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.4% [-0.5%, -0.1%] 8
All ❌✅ (primary) 0.3% [0.2%, 0.5%] 14

Max RSS (memory usage)

Results (secondary -5.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-5.5% [-5.5%, -5.5%] 1
All ❌✅ (primary) - - 0

Cycles

Results (primary -2.8%, secondary -3.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.8% [-2.8%, -2.8%] 1
Improvements ✅
(secondary)
-3.2% [-3.2%, -3.2%] 1
All ❌✅ (primary) -2.8% [-2.8%, -2.8%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 487.064s -> 481.364s (-1.17%)
Artifact size: 397.85 MiB -> 395.69 MiB (-0.54%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 19, 2026
@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2026

Zalathar is not on the review rotation at the moment.
They may take a while to respond.

@rustbot
Copy link
Collaborator

rustbot commented Feb 22, 2026

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@nnethercote
Copy link
Contributor Author

I rebased.

@Zalathar
Copy link
Member

`SemiDynamicQueryDispatcher` is just a `QueryVTable` wrapper with an
additional `const FLAGS: QueryFlags` generic parameter that contains
three booleans. This arrangement exists as a performance optimization.
But the performance effects are very small and it adds quite a bit of
complexity to an already overly-complex part of the codebase. If it
didn't exist and somebody proposed adding it and asked me to review, I
almost certainly wouldn't approve it.

This commit removes it. The three booleans in `QueryFlags` are moved
into `QueryVTable` The non-trivial methods of
`SemiDynamicQueryDispatcher` become methods of `QueryVTable`.
It's now `query_vtable` because its return type changed. And thanks to
the previous commit it can be manually inlined in several places. (The
only remaining calls to it are in `make_dep_kind_vtable_for_query`,
which are more challenging to remove.)
@nnethercote
Copy link
Contributor Author

@bors r=Zalathar

@rust-bors
Copy link
Contributor

rust-bors bot commented Feb 22, 2026

📌 Commit 5aebfd6 has been approved by Zalathar

It is now in the queue for this repository.

@rust-bors rust-bors bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 22, 2026
@Zalathar
Copy link
Member

Scheduling: Giving this a bump as it's on the critical path for any subsequent work involving query vtables.

@bors p=1

@rust-bors

This comment has been minimized.

rust-bors bot pushed a commit that referenced this pull request Feb 23, 2026
Remove `const FLAGS`.

*[View all comments](https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rust/pull/152791)*

The performance wins provided by these types are meagre, and I don't think they justify the code complexity they introduce.

r? @Zalathar
@JonathanBrouwer
Copy link
Contributor

@bors cancel
x86_64-gnu-aux looks super stuck

@rust-bors
Copy link
Contributor

rust-bors bot commented Feb 23, 2026

Auto build cancelled. Cancelled workflows:

The next pull request likely to be tested is #153013.

@rust-bors

This comment has been minimized.

@rust-bors rust-bors bot added merged-by-bors This PR was explicitly merged by bors. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 23, 2026
@rust-bors
Copy link
Contributor

rust-bors bot commented Feb 23, 2026

☀️ Test successful - CI
Approved by: Zalathar
Duration: 3h 20m 56s
Pushing b3869b9 to main...

@rust-bors rust-bors bot merged commit b3869b9 into rust-lang:main Feb 23, 2026
12 checks passed
@rustbot rustbot added this to the 1.95.0 milestone Feb 23, 2026
@github-actions
Copy link
Contributor

What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing eeb94be (parent) -> b3869b9 (this PR)

Test differences

Show 2 test diffs

2 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard b3869b94cd1ed4bfa2eb28f301535d5e9599c713 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. dist-apple-various: 1h 41m -> 2h 18m (+36.0%)
  2. dist-aarch64-llvm-mingw: 1h 25m -> 1h 45m (+23.7%)
  3. dist-aarch64-apple: 2h 4m -> 2h 25m (+16.9%)
  4. aarch64-gnu-debug: 1h 16m -> 1h 6m (-13.6%)
  5. i686-gnu-2: 1h 45m -> 1h 31m (-13.6%)
  6. x86_64-gnu-stable: 2h 26m -> 2h 7m (-12.6%)
  7. x86_64-msvc-ext2: 1h 48m -> 1h 36m (-11.6%)
  8. x86_64-gnu-gcc: 1h 8m -> 1h (-10.6%)
  9. aarch64-gnu-llvm-20-1: 1h 2m -> 55m 45s (-10.5%)
  10. armhf-gnu: 1h 30m -> 1h 21m (-10.5%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b3869b9): comparison URL.

Overall result: ❌ regressions - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

  • If the regression was expected or you think it can be justified,
    please write a comment with sufficient written justification, and add
    @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
  • If you think that you know of a way to resolve the regression, try to create
    a new PR with a fix for the regression.
  • If you do not understand the regression or you think that it is just noise,
    you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
    were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
0.2% [0.1%, 0.5%] 17
Regressions ❌
(secondary)
0.3% [0.1%, 0.6%] 37
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [0.1%, 0.5%] 17

Max RSS (memory usage)

Results (primary -1.4%, secondary -5.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.4% [-1.4%, -1.4%] 1
Improvements ✅
(secondary)
-5.3% [-5.3%, -5.3%] 1
All ❌✅ (primary) -1.4% [-1.4%, -1.4%] 1

Cycles

Results (secondary 8.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
8.6% [8.6%, 8.6%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 481.498s -> 479.41s (-0.43%)
Artifact size: 395.88 MiB -> 395.68 MiB (-0.05%)

@nnethercote
Copy link
Contributor Author

Perf regressions were minor and deemed worthwhile above for the simplicity improvements.

@rustbot label: +perf-regression-triaged

@nnethercote nnethercote deleted the rm-const-FLAGS branch February 23, 2026 23:28
@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Feb 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants